[APMSVLS-469] feat(traces): rescue errored traces via agent-side error sampler - #1320
[APMSVLS-469] feat(traces): rescue errored traces via agent-side error sampler#1320lucaspimentel wants to merge 13 commits into
Conversation
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Adds an agent-side error sampler to the Bottlecap trace processing pipeline so that errored traces that were auto-dropped (P0/AutoDrop) on the lambda_extension_compute_stats path can be “rescued” up to a configurable TPS budget, improving error visibility while still keeping non-errored P0 traces dropped and honoring explicit user drops.
Changes:
- Introduces an
ErrorsSamplerintoServerlessTraceProcessorand uses it to selectively retain erroredAutoDropchunks, stamping_dd.errors_sron rescued roots. - Adds config surface area for
DD_APM_ERROR_TPSandDD_APM_EXTRA_SAMPLE_RATE(defaults + env/YAML tests). - Wires the sampler into the runtime entrypoint and updates unit/integration tests and third-party licensing/deps to include the new shared sampler crate.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| bottlecap/src/traces/trace_processor.rs | Implements the rescue decision path for errored AutoDrop chunks and adds unit tests for the new behavior. |
| bottlecap/src/config/mod.rs | Adds apm_error_tps / apm_extra_sample_rate configuration fields and parsing/tests. |
| bottlecap/src/bin/bottlecap/main.rs | Constructs and injects the error sampler into the trace processor using config values. |
| bottlecap/tests/apm_integration_test.rs | Updates the integration pipeline wiring to include the new sampler field. |
| bottlecap/src/lifecycle/invocation/processor.rs | Updates test constructions of ServerlessTraceProcessor to provide an error sampler. |
| bottlecap/Cargo.toml | Adds the datadog-agent-trace-sampler dependency and updates serverless-components rev pins. |
| bottlecap/Cargo.lock | Locks the new sampler crate and updates the serverless-components sources to the new rev. |
| bottlecap/LICENSE-3rdparty.csv | Adds third-party license metadata for datadog-agent-trace-sampler. |
On the lambda_extension_compute_stats path, the extension drops every trace marked P0 (priority <= 0) after computing its stats. This adds an error sampler that gives those dropped traces a second look: errored traces are kept (rescued) up to DD_APM_ERROR_TPS traces/sec (default 10), distributed fairly across trace signatures, with _dd.errors_sr stamped on the rescued root span. Non-errored P0 traces are still dropped, and stats still count all traces. This guarantees error visibility even under aggressive sampling. Ports the Go trace agent's ScoreSampler/ErrorTPS behavior via the shared, dependency-free datadog-agent-trace-sampler crate. New config: - DD_APM_ERROR_TPS (default 10.0; 0 disables the rescue) - DD_APM_EXTRA_SAMPLE_RATE (default 1.0) 🤖
Errored traces were only rescued from a drop decision when the root span itself carried the error, so a trace whose failure happened deeper (for example a failed downstream call that the handler caught) was still dropped. Now an error anywhere in the trace makes it a rescue candidate, matching the Datadog Agent. 🤖
Traces dropped on purpose, either by a tracer sampling rule or by an explicit MANUAL_DROP, were being fed to the error sampler and could be sent to Datadog anyway when they contained an error. Only traces dropped by automatic sampling are now rescue candidates, matching the Datadog Agent. 🤖
The error sampler's per-signature rate limits were keyed on the extension's own DD_ENV, so when that was unset every trace shared one empty env and distinct services competed for the same budget. The env the tracer reported with the trace is now used instead, matching the Datadog Agent. 🤖
A panic while the error sampler's lock was held poisoned the mutex, which silently disabled error-trace rescue for the rest of the sandbox's life. Recover through poisoning instead: the sampler holds only rolling-window counters, so a partially updated bucket costs far less than losing the feature entirely. 🤖
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The disabled check re-derived "sampler is off" from apm_error_tps at the call site, a second definition of a condition the sampler already knows. It now asks the sampler via the new is_disabled(), hoisted above the payload loop so it costs one lock per flush instead of a check per chunk. Repins the four serverless-components deps to pick up is_disabled(), which also brings in non-finite client sample rate handling. 🤖
7a0f64f to
328cf05
Compare
Bump the serverless-components pin to 54e570ae, which adds the dual-mode ErrorSamplerMode (AlwaysKeep | RateLimited) to datadog-agent-trace-sampler and makes `mode` a required field on ErrorSamplerConfig. The rev also carries a fix for non-finite client sample rates on the error sample rate path. Hardcode the production sampler to AlwaysKeep: Lambda's per-invocation trace volume is low, so the RateLimited budget rarely binds, and freeze/thaw breaks its 30s wall-clock window. Wiring the mode through config is deferred to a follow-up. Correct the doc comments on apm_error_tps, apm_extra_sample_rate, and ServerlessTraceProcessor::error_sampler, which described RateLimited behavior that no longer runs in production. Refs APMSVLS-469
Replace apm_error_tps and apm_extra_sample_rate with a single apm_error_sampler_enabled toggle (DD_APM_ERROR_SAMPLER_ENABLED). Both replaced knobs were introduced earlier on this branch and never released, so there is no compatibility constraint. Neither carries its advertised meaning in AlwaysKeep mode: extra_sample_rate is ignored outright, and error_tps degrades to an on/off switch, so DD_APM_ERROR_TPS=25 and =1 behave identically. Exposing a rate cap that is not enforced is worse than not exposing one. Both return, with their real semantics, when RateLimited is wired up. Default false while the feature rolls out as opt-in; the plan is to flip it once it has soaked. AlwaysKeep derives its disabled flag from target_tps <= 0.0, so the boolean maps onto 1.0 / 0.0 and the disabled path short-circuits before any SpanView is built. Refs APMSVLS-469
Build the shipped error sampler (AlwaysKeep, enabled by apm_error_sampler_enabled) in one place so tests exercise the same configuration that ships, and cover the disabled default with a test. A failed clock read no longer aborts the extension, and the clock is only read when error rescue is enabled. In AlwaysKeep mode the sampler ignores span contents, so only the root span view is built instead of one per span. 🤖
The float_cmp allow no longer matches any comparison in the test module and would mask a real one. 🤖
Condense the doc and inline comments added with the error sampler: drop roadmap notes, references to prior behavior, and comments that restate the code they sit above. 🤖
TL;DR
Rescues errored traces that automatic sampling drops on the
lambda_extension_compute_statspath, so errors stay visible even under aggressive sampling. Ships disabled behindDD_APM_ERROR_SAMPLER_ENABLED, using theAlwaysKeepmode of the newdatadog-agent-trace-samplercrate (DataDog/serverless-components#141). Draft until that crate merges.Overview
On the
lambda_extension_compute_statspath, the extension drops every P0 (priority <= 0) trace after computing its stats, so an errored trace the tracer sampled away is invisible in the UI even though its stats are counted. This adds an agent-side error sampler that rescues those traces and stamps_dd.errors_sron the root span, matching the Go trace agent'sScoreSampler/ErrorTPSbehavior.Candidates, matching the Go agent:
MANUAL_DROP) are always honored.The sampling logic lives in the dependency-free
datadog-agent-trace-samplercrate (DataDog/serverless-components#141): primitives in (SpanView/TraceView), aSampleDecisionout, no protobufSpantype, so consumers pinning different libdatadog revisions can share it.Configuration
DD_APM_ERROR_SAMPLER_ENABLEDfalseFlat key (
apm_error_sampler_enabled), unlike the Go agent's nestedapm_config.*.The crate offers two strategies; this PR ships
AlwaysKeep(rescue every errored auto-dropped trace), since Lambda's per-invocation volume is low and freeze/thaw breaksRateLimited's 30s window.RateLimited(a traces/sec budget à la the Go agent'serrors_per_second) stays unwired, along withDD_APM_ERROR_TPS/DD_APM_EXTRA_SAMPLE_RATE.When disabled, the rescue path — including the clock read — is skipped entirely, so behavior is unchanged from before this PR. When enabled, a failed clock read now falls back to
0instead of aborting the extension.Blocked on
DataDog/serverless-components#141: the four pins in
bottlecap/Cargo.tomlpoint at its branch rev (54e570a) and need repinning tomainonce merged.Testing
Unit tests in
traces/trace_processor.rs:test_error_sampler_rescues_errored_p0_chunks— errored auto-dropped chunk is rescued and stamped; non-errored stays dropped; explicit user drop is never rescued.test_error_sampler_rescues_chunk_with_errored_child_span— a healthy root with an errored child is still rescued.test_disabled_error_sampler_drops_errored_p0_chunks— shipping default (disabled) still drops errored P0 traces.Also: config tests for
apm_error_sampler_enabled's default/overrides;apm_integration_test.rsruns the existing APM assertions with the sampler enabled; prod and tests share one constructor (new_error_sampler) so tests exercise the shipping config.cargo test— 567 passed.cargo clippy --all-targetsandcargo fmt --checkclean.🤖